Skip to content

[WIP] Improve knightos/display.h#23

Open
aviallon wants to merge 3 commits into
KnightOS:masterfrom
aviallon:patch-display-improvement
Open

[WIP] Improve knightos/display.h#23
aviallon wants to merge 3 commits into
KnightOS:masterfrom
aviallon:patch-display-improvement

Conversation

@aviallon
Copy link
Copy Markdown
Member

@aviallon aviallon commented Jun 6, 2020

Implemented several functions in C since I don't know if the kernel supports it natively.

Implemented in C since I don't know if the kernel supports it natively.
@aviallon aviallon changed the title [WIP] Improve knightos/display.h Improve knightos/display.h Jun 6, 2020
@aviallon aviallon marked this pull request as ready for review June 6, 2020 01:17
Copy link
Copy Markdown

@pixelherodev pixelherodev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, it'd probably be better to write this in asm and expose a small C interface. SDCC 4 is a lot better with codegen, but it still reeks compared to hand-written (and we're not on it, just yet).

If you'd rather not redo this, I get it, and I'm happy to merge as is after testing a bit.

Comment thread src/knightos/display.c Outdated
Comment thread src/knightos/display.c
}

void draw_float(SCREEN* screen, unsigned char x, unsigned char y, float value){
/* Implementation is weird and slow because of non-working snprintf. Indeed, %f format strings are not yet implemented */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviallon what was wrong with snprintf?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%f doesn't work at all (it's ignored). I don't know if it is standard, but every libc I know supports it. And it really is useful.

Copy link
Copy Markdown
Member

@MaxLeiter MaxLeiter Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why we didn't implement %f, and there's probably a good reason, but you may want to experiment with https://github.com/KnightOS/libc/blob/master/src/format.c

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worst case, I'd like to see %f support using this function as a base.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this function has a major issue :
it does not support floats whose integer part is greater than INT_MAX...
In order to have full float support, I would have to go char by char... which may be even slower 😞

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this PR is done.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I will do. If you have any idea on a fast implementation for that, I'm all ears 😝

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First idea: don't worry about fast. It can be sped up later if we need to.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this is ready for another review :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps in a few other years 😆

@aviallon
Copy link
Copy Markdown
Member Author

aviallon commented Jun 6, 2020

I may try to do the signed integer one in ASM. But for floats it may be a little hard.

@MaxLeiter
Copy link
Copy Markdown
Member

@aviallon fyi @pixelherodev and I are active on IRC. #knightos on freenode.net

@aviallon
Copy link
Copy Markdown
Member Author

aviallon commented Jun 6, 2020

@aviallon fyi @pixelherodev and I are active on IRC. #knightos on freenode.net

Are you interested in a Matrix group? (IRC bridges are easy to set up and officially supported)

I'm coming soon.

@pixelherodev pixelherodev changed the title Improve knightos/display.h [WIP] Improve knightos/display.h Jun 12, 2020
@pixelherodev
Copy link
Copy Markdown

Needs to be updated now that e.g. log10 is in libc.

@aviallon
Copy link
Copy Markdown
Member Author

aviallon commented Jun 14, 2020

WTF happened (somehow I had random commits in my PR)

@aviallon aviallon force-pushed the patch-display-improvement branch from 6355594 to a35754c Compare June 14, 2020 20:03
@aviallon aviallon requested a review from pixelherodev June 15, 2020 08:19
@aviallon
Copy link
Copy Markdown
Member Author

Updated to use log10u. I am in the process to try to make a draw_float able to display any floats, but it certainly is not trivial ! I got some ideas though...

Comment thread include/knightos/display.h

/**
* Draws a long at x, y
* NYI
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO is better, it's more commonly used

Comment thread src/knightos/display.c
value = -value;
}
draw_short(screen, x, y, value);
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the explicit return?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reaching the end of a function returning void is equivalent to return;. Reaching the end of any other value-returning function is undefined behavior if the result of the function is used in an expression (it is allowed to discard such return value). For main, see main function.
https://cppreference.com/w/c/language/return.html

I got bitten by non-standard conforming compilers that behaved incorrectly with void functions in the past.

Comment thread src/knightos/display.c
}

void draw_float(SCREEN* screen, unsigned char x, unsigned char y, float value){
/* Implementation is weird and slow because of non-working snprintf. Indeed, %f format strings are not yet implemented */
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this is ready for another review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants